Repository navigation
fix(sqlalchemy-spanner): isolate literal round trip test tables with unique naming and teardown - #18623
fix(sqlalchemy-spanner): isolate literal round trip test tables with unique naming and teardown#18623chalmerlowe wants to merge 2 commits into
Conversation
…unique naming and teardown
Prevent schema collisions ('400 Duplicate name in schema') across test
invocations by generating unique table names per test execution and
guaranteeing table teardown with committed DDL and MetaData eviction.
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request enhances test isolation in the literal_round_trip_spanner fixture by generating unique table names using UUIDs, explicitly committing DDL operations, and adding a finally block to drop the table and remove it from metadata. The review feedback suggests three improvements: using dbapi_connection with a fallback to support SQLAlchemy 2.0 compatibility, moving the table creation inside the try block to ensure cleanup if creation fails, and wrapping the table drop in a nested try...finally block to guarantee that the table is always removed from metadata even if the drop operation itself raises an exception.
|
|
||
| def _commit_ddl(connection): | ||
| """Flush and commit buffered DDL in Spanner DBAPI or underlying connections.""" | ||
| dbapi_conn = getattr(connection, "connection", connection) |
There was a problem hiding this comment.
In SQLAlchemy 2.0, Connection.connection is deprecated in favor of Connection.dbapi_connection. To ensure compatibility with both SQLAlchemy 1.4 and 2.0 without triggering deprecation warnings, retrieve dbapi_connection first, falling back to connection if it is not present.
| dbapi_conn = getattr(connection, "connection", connection) | |
| dbapi_conn = getattr(connection, "dbapi_connection", getattr(connection, "connection", connection)) |
| t.create(connection) | ||
|
|
||
| for value in input_: | ||
| ins = t.insert().values(x=literal(value, type_)) | ||
| connection.execute(ins) | ||
|
|
||
| if support_whereclause: | ||
| if compare: | ||
| stmt = t.select().where( | ||
| t.c.x | ||
| == literal( | ||
| compare, | ||
| type_, | ||
| ), | ||
| t.c.x | ||
| == literal( | ||
| input_[0], | ||
| type_, | ||
| ), | ||
| ) | ||
| else: | ||
| stmt = t.select().where( | ||
| t.c.x | ||
| == literal( | ||
| compare if compare is not None else input_[0], | ||
| type_, | ||
| _commit_ddl(connection) | ||
|
|
||
| try: |
There was a problem hiding this comment.
If t.create(connection) fails or raises an exception, the try...finally block is never entered. Since Table automatically registers itself in the metadata object upon instantiation, this leaves a stale table reference in metadata and can leak the table in the database if the creation partially succeeded. Wrapping the table creation inside the try block ensures that the cleanup in the finally block is always executed.
| t.create(connection) | |
| for value in input_: | |
| ins = t.insert().values(x=literal(value, type_)) | |
| connection.execute(ins) | |
| if support_whereclause: | |
| if compare: | |
| stmt = t.select().where( | |
| t.c.x | |
| == literal( | |
| compare, | |
| type_, | |
| ), | |
| t.c.x | |
| == literal( | |
| input_[0], | |
| type_, | |
| ), | |
| ) | |
| else: | |
| stmt = t.select().where( | |
| t.c.x | |
| == literal( | |
| compare if compare is not None else input_[0], | |
| type_, | |
| _commit_ddl(connection) | |
| try: | |
| try: | |
| t.create(connection) | |
| _commit_ddl(connection) |
| finally: | ||
| t.drop(connection, checkfirst=True) | ||
| _commit_ddl(connection) | ||
| metadata.remove(t) |
There was a problem hiding this comment.
If t.drop(connection) or _commit_ddl(connection) raises an exception (e.g., due to connection issues), metadata.remove(t) will be bypassed, leaving a stale table reference in metadata. Wrapping the drop in a nested try...finally guarantees that metadata.remove(t) is always executed.
| finally: | |
| t.drop(connection, checkfirst=True) | |
| _commit_ddl(connection) | |
| metadata.remove(t) | |
| finally: | |
| try: | |
| t.drop(connection, checkfirst=True) | |
| _commit_ddl(connection) | |
| finally: | |
| metadata.remove(t) |
This branch has not been deployed
1 participant
Fix Schema Collision and DDL Concurrency in SQLAlchemy Spanner Literal Tests
Problem
In the Cloud Spanner SQLAlchemy compliance tests, tests using the
literal_round_trip_spannerfixture were failing with400 Duplicate name in schema: t_literal_round_trip_spanner.This occurred because the fixture created a table with a static, hardcoded name (
t_literal_round_trip_spanner) without teardown between tests. When subsequent test methods in the same class (such asStringTest,NumericTest,BooleanTest, andTextTest) invoked the fixture, Cloud Spanner rejected creating an already existing table. Furthermore, executing DDL statements while read-write transactions from queries remain active triggers400 Schema change operation rejected because a concurrent schema change operation or read-write transaction is already in progress.Solution
t_lit_string_a3f2b1).dbapi_connectionfirst with fallback toconnectionto support both modern SQLAlchemy 2.0 and legacy 1.4 without deprecation warnings.try...finallyblock to ensure that table entries are always evicted from SQLAlchemy metadata even if creation or drop operations fail.Related Pull Requests
This fix unblocks and resolves failing
Kokoro Sqlalchemy Compliance Testsacross the following pull requests once rebased ontomain:chore(deps): update dependency mako to v1.4.2 [security])chore(deps): update dependency urllib3 to v2.6.3)chore(deps): update dependency packaging to v26.3)chore(deps): update dependency pycparser to v3.11)Notes to Reviewers
packages/sqlalchemy-spanner/tests/conftest.py.